Skip to content

Deprecate DatabaseJanitor version argument - #1405

Open
w3lld1 wants to merge 1 commit into
dbfixtures:mainfrom
w3lld1:fix/deprecate-janitor-version
Open

Deprecate DatabaseJanitor version argument#1405
w3lld1 wants to merge 1 commit into
dbfixtures:mainfrom
w3lld1:fix/deprecate-janitor-version

Conversation

@w3lld1

@w3lld1 w3lld1 commented Aug 10, 2026

Copy link
Copy Markdown

Summary

  • I made the unused version argument optional on both janitor classes and emit a DeprecationWarning when callers still provide it.
  • I removed the argument from the plugin's internal janitor construction paths and from the public examples.
  • I kept supplied values available during the deprecation period, added regression coverage, and recorded the deprecation in a newsfragment. A separate follow-up can remove the compatibility path after the deprecation window.

Validation

  • uvx pre-commit run --from-ref upstream/main --to-ref HEAD
  • uv run pytest tests/test_janitor.py -q -k 'not test_janitor_populate_async_sql_path and not test_async_janitor_init_and_drop and not test_async_janitor_template_flag_and_context_manager and not test_async_janitor_creates_database_from_template' (31 passed)
  • uv run pytest tests/test_factory_errors.py -q (3 passed)
  • uv build
  • git diff --check upstream/main...HEAD

The four PostgreSQL-backed janitor tests are deferred to CI because this environment does not provide pg_config or a PostgreSQL server.

Fixes #1393

Summary by CodeRabbit

  • Deprecation

    • The optional PostgreSQL version argument is now deprecated when supplied to database janitors.
    • Existing version values remain supported and generate a deprecation warning.
  • Documentation

    • Usage examples have been updated to omit the deprecated argument.
  • Improvements

    • Database janitors now work without requiring an explicit PostgreSQL version, simplifying synchronous and asynchronous setup.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The janitor version argument is now optional and deprecated. Production fixtures, examples, and tests no longer pass it by default. Supplied values still trigger a warning and retain version parsing behaviour.

Changes

Janitor version deprecation

Layer / File(s) Summary
Optional and deprecated API
pytest_postgresql/janitor.py, newsfragments/1393.depr.rst
The janitor accepts version=None, types the public attribute as optional, and warns when a version is supplied.
Fixture and example updates
pytest_postgresql/factories/client.py, pytest_postgresql/factories/noprocess.py, pytest_postgresql/factories/process.py, README.rst
Janitor construction no longer passes the PostgreSQL process version.
Test coverage updates
tests/test_janitor.py, tests/test_noopexecutor.py, tests/test_postgres_options_plugin.py
Tests cover optional construction, deprecation warnings, version conversion, and operations without an explicit version.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: fizyk, tboy1337

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: deprecating the DatabaseJanitor version argument.
Linked Issues check ✅ Passed The changes satisfy issue #1393 by deprecating the version argument, making it optional, warning on use, and preserving compatibility.
Out of Scope Changes check ✅ Passed The changes remain within scope and support deprecation through code updates, documentation, tests, and a newsfragment.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Newsfragment Check ✅ Passed Added newsfragments/1393.depr.rst; depr is a valid towncrier type, and the pull request includes substantive code and test changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_janitor.py`:
- Around line 24-27: Replace the Any annotation on version in both deprecation
tests with str | float | Version, and change the parametrized numeric value 10
to 10.0 so it matches the supported input type and BaseDatabaseJanitor.__init__
contract.
- Line 37: Update the version values in the pytest.mark.parametrize decorator to
use a list instead of a tuple, while preserving VERSION, 10, and "10" as the
parameter cases.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3014c437-3346-468e-b559-cc97cffee6cf

📥 Commits

Reviewing files that changed from the base of the PR and between e72b4ef and b023103.

📒 Files selected for processing (9)
  • README.rst
  • newsfragments/1393.depr.rst
  • pytest_postgresql/factories/client.py
  • pytest_postgresql/factories/noprocess.py
  • pytest_postgresql/factories/process.py
  • pytest_postgresql/janitor.py
  • tests/test_janitor.py
  • tests/test_noopexecutor.py
  • tests/test_postgres_options_plugin.py
💤 Files with no reviewable changes (6)
  • README.rst
  • pytest_postgresql/factories/client.py
  • tests/test_postgres_options_plugin.py
  • pytest_postgresql/factories/noprocess.py
  • pytest_postgresql/factories/process.py
  • tests/test_noopexecutor.py

Comment thread tests/test_janitor.py
Comment on lines +24 to +27
def test_version_is_deprecated(version: Any) -> None:
"""Test that version is cast to Version object with a deprecation warning."""
with pytest.warns(DeprecationWarning, match="version argument is deprecated"):
janitor = DatabaseJanitor(user="user", host="host", port="1234", dbname="database_name", version=version)

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace Any with the supported version input type.

Ruff reports ANN401 for version in both deprecation tests. Use a concrete union that matches the parametrised values and BaseDatabaseJanitor.__init__, such as changing 10 to 10.0 and annotating str | float | Version. This preserves static checking.

Also applies to: 39-42

🧰 Tools
🪛 Ruff (0.16.1)

[warning] 24-24: Dynamically typed expressions (typing.Any) are disallowed in version

(ANN401)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_janitor.py` around lines 24 - 27, Replace the Any annotation on
version in both deprecation tests with str | float | Version, and change the
parametrized numeric value 10 to 10.0 so it matches the supported input type and
BaseDatabaseJanitor.__init__ contract.

Source: Linters/SAST tools

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai file an issue to enable, it's not enabled at the moment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="dbfixtures/pytest-postgresql"
title="Enable Ruff ANN401 checks"
body=$(cat <<'EOF'
## Summary

Enable Ruff rule `ANN401` in the repository configuration.

## Rationale

The rule detected `typing.Any` in parameter annotations in `tests/test_janitor.py`. The check is not currently enabled, so it cannot prevent similar annotations in future changes.

## Affected areas

- Ruff configuration
- Python type annotations, including `tests/test_janitor.py`

## Required changes

Enable `ANN401`, or the relevant Ruff annotation rule set that includes it, in the project lint configuration. Confirm that the configured checks run in the standard lint workflow.

## Acceptance criteria

- Ruff reports `ANN401` for parameters annotated as `typing.Any`.
- The repository lint configuration enables the rule.
- The standard lint workflow executes the enabled rule.

## Backlinks

- Pull request: https://github.com/dbfixtures/pytest-postgresql/pull/1405
- Review comment: https://github.com/dbfixtures/pytest-postgresql/pull/1405#discussion_r3748701045
- Requested by: `@fizyk`
EOF
)

gh issue create \
  --repo "$repo" \
  --title "$title" \
  --body "$body" \
  --assignee fizyk

Length of output: 2743


@fizyk, I created and assigned #1406: Enable Ruff ANN401 checks.


🧬 Code Graph Analysis Results

pytest_postgresql/janitor.py

  • Lines 222–337 — Complete AsyncDatabaseJanitor implementation, including init(), drop(), template handling, SQL loading, async cursor management, and async context-manager lifecycle methods.
  • Lines 240–253AsyncDatabaseJanitor.drop() checks database existence, disables connections, terminates active connections, clears the template flag when needed, and drops the database.
  • Lines 298–323AsyncDatabaseJanitor.cursor() connects to the configured maintenance database, applies isolation/autocommit settings, yields an async cursor, and closes the connection during cleanup.
  • Lines 268–295AsyncDatabaseJanitor.load() builds the loader with sql_async, forwards connection parameters and autocommit, and handles synchronous, asynchronous, and awaitable-returning loaders.

pytest_postgresql/factories/process.py

  • Lines 85–244postgresql_proc() fixture factory creates and starts a PostgreSQL executor, configures DatabaseJanitor, initializes the template database, loads configured data, and performs cleanup.
  • Lines 198–214 — The fixture passes load_autocommit configuration or override to DatabaseJanitor(autocommit=...), then calls janitor.drop(), janitor.init(), and janitor.load(...).

You are interacting with an AI system.

Comment thread tests/test_janitor.py
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DatabaseJanitor version is a dead code

2 participants